-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BIOMAGE-1989]- Reduce UI build time #770
Conversation
📦 Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action 🤖 This PR introduced no changes to the javascript bundle 🙌 |
Codecov Report
@@ Coverage Diff @@
## master #770 +/- ##
=======================================
Coverage 83.40% 83.40%
=======================================
Files 462 462
Lines 7850 7850
Branches 1536 1536
=======================================
Hits 6547 6547
Misses 1249 1249
Partials 54 54 Continue to review full report at Codecov.
|
- id: setup-buildx | ||
name: Set up Docker Buildx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw from the error in this run: https://github.com/hms-dbmi-cellenics/ui/runs/7291171952?check_suite_focus=true and read about it from the docker/build-push-action
section about driver: https://github.com/docker/build-push-action#usage
cache-from: type=gha | ||
cache-to: type=gha,mode=max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did you work out that we need to have these parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned that the docker/build-push-action has internal caching capabilities and we're not using it. It has an option for github actions, and I'm following the examples here: https://github.com/docker/build-push-action/blob/master/docs/advanced/cache.md#github-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, I just have some questions about the setup
Description
Comments
Looking at https://github.com/hms-dbmi-cellenics/ui/actions/runs/2649477889, most of the CI time is spent on
I learned that the caches we use do not cache
node_modules
and it is not recommended to do so. What most caches do is cache the.npm
cache. This means thatnpm run ci
builds from this cache, and the time it takes to build (13m) is the installation time using the.npm
cache. The modules do have somepostintallation
scripts. I have tried disabling it, but it doesn't make any difference. From this, I think we can't cut down npm installs further.I found out that a step in the Dockerfile also involves installs the NPM modules. The good news is, we can cache intermediate Docker layers and use this cache to build on. This is the main change in this PR. From this run: https://github.com/hms-dbmi-cellenics/ui/actions/runs/2653367417, caching the intermediate layer cuts Docker build time from 22 mins to 3 mins.
Details
URL to issue
https://biomage.atlassian.net/browse/BIOMAGE-1989
https://ui-agi-cozy-eel.scp-staging.biomage.net/
Link to staging deployment URL (or set N/A)
N/A
Links to any PRs or resources related to this PR
N/A
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional